fix(datastore): preserve staged deletes across Merge composition#1044
Merged
giogam merged 3 commits intoJun 12, 2026
Conversation
MemoryDataStore.Merge propagated src.DeletedRemoteKeys by calling dst.<Store>.Delete(key), which removed the record from dst.Records but never appended the key to dst.DeletedRemoteKeys. Chained Merges (operation and changeset composition) consumed the prior step's staged deletes at every hop; both the JSON file and catalog persistence backends drive remote deletes off DeletedRemoteKeys, so any delete that passed through more than one Merge silently never reached the remote. Each of the three propagation blocks (AddressRef, ChainMetadata, ContractMetadata) now calls dst.<Store>.RemoteDelete(key) followed by dst.<Store>.Delete(key) with the per-store NotFound sentinel tolerated. Delete intent now survives chained Merges, and Merge is idempotent on repeated calls with the same source. Behavior change: Merge previously returned Err<X>NotFound when the source staged a delete for a key not present in dst.Records; it now succeeds and stages the delete on dst. This aligns observable behavior with the RemoteDelete docstring, which already advertised staging deletes for records not held locally. Three existing tests in memory_datastore_test.go are inverted accordingly; new tests cover chained-merge preservation, idempotency, and the precedence of source-live records over destination-staged deletes.
🦋 Changeset detectedLatest commit: 0e33953 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Fixes the datastore to ensure that staged deletes are preserved during merge composition.
graham-chainlink
previously approved these changes
Jun 12, 2026
Comment on lines
+80
to
+86
| // Precedence: a live record in other.<Store>.Records overrides a staged delete in | ||
| // s.<Store>.DeletedRemoteKeys for the same key. This is a side-effect of Upsert, | ||
| // which clears the key from DeletedRemoteKeys whenever a record is upserted (so a | ||
| // direct `RemoteDelete(k); Upsert(rec-with-k)` cancels the staged delete on the | ||
| // same store). Staged deletes are only "sticky" on the source side. Callers that | ||
| // need a destination-side staged delete to survive Merge must ensure the source | ||
| // either omits the live record or stages the delete itself. |
Collaborator
There was a problem hiding this comment.
similar here too? Do users need to know all this before using it? I find readin this kind of overwhelming :D
Contributor
Author
There was a problem hiding this comment.
I think this context is important for explaining the behavior of Merge(), as some aspects of its implementation can otherwise appear opaque or difficult to understand.
ajaskolski
previously approved these changes
Jun 12, 2026
|
ajaskolski
approved these changes
Jun 12, 2026
giogam
deleted the
CLD-2758/memory-data-store-merge-silently-dropping-staged-deletes-across-composition
branch
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
MemoryDataStore.Mergepropagatedsrc.DeletedRemoteKeysby callingdst.<Store>.Delete(key), which removed the record fromdst.Recordsbut never appended the key todst.DeletedRemoteKeys. Chained Merges (operation and changeset composition) consumed the prior step's staged deletes at every hop; both the JSON file and catalog persistence backends drive remote deletes off DeletedRemoteKeys, so any delete that passed through more than one Merge silently never reached the remote.Each of the three propagation blocks (
AddressRef,ChainMetadata,ContractMetadata) now callsdst.<Store>.RemoteDelete(key)followed bydst.<Store>.Delete(key)with the per-storeNotFoundsentinel tolerated. Delete intent now survives chained Merges, and Merge is idempotent on repeated calls with the same source.Behavior change: Merge previously returned ErrNotFound when the source staged a delete for a key not present in
dst.Records; it now succeeds and stages the delete on dst. This aligns observable behavior with the RemoteDelete docstring, which already advertised staging deletes for records not held locally. Three existing tests inmemory_datastore_test.goare inverted accordingly; new tests cover chained-merge preservation, idempotency, and the precedence of source-live records over destination-staged deletes.